Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make 'QuaiHDWallet' class to extend 'HDWallet' #151

Conversation

alejoacosta74
Copy link
Contributor

This PR changes the following:

  • Moves properties from QuaiHDWallet class to parent HDWallet class
  • Implements the getAddress method for QuaiHDWallet
  • normalizes some names aligned with Qi and Quai ledgers.

@alejoacosta74 alejoacosta74 force-pushed the ft/quaiwallet-getaddress-rebase branch from 7fadcb6 to f37ad6a Compare May 22, 2024 17:49
@rileystephens28
Copy link
Member

@alejoacosta74 pretty much everything in this PR looks good and implements what we have previously agreed on. However, at some point I think we may have introduced an error in the mental model we use regarding the purpose of HD Wallets as they are implemented in ethers/quais. I am going to outline my thoughts below on how I believe they were intended to work and how our updates may go against the use case.

Let's start by looking at how the ethers package intended the HD wallet to be used. First, you will notice it is called an HDNodeWallet implying that each instance is supposed to represent a node within the derivation tree. When viewed through this lens, it becomes more clear the purpose of deriveChild() and there is no deriveAddress() present in the ethers implementation of HDNodeWallet. This is because an instance of an HD node does not require a full path, but rather supplies the method deriveChild() that allows for reaching an HD node that ultimately has a complete BIP44 path. Let's look at an example - a master HD node for deriving Quai addresses should be able to accept the path m/44/969, where the account, change, and address_index values are not yet specified. This master node instance could then create a child node instance for account 0 by calling master_quai_node.deriveChild(0) which would create a node with a resulting path of m/44/969/0. Still, at this depth we are not able to generate a meaningful address that would be used within the Quai ledger of any shard because we still must specify change and address_index values in the path. The following series of operations would get us to a point where we would have a node whose address created from the public key would (sharded address space aside) be correct according to BIP44 and thus usable within a wallet implementation:

account_0_node = master_quai_node.deriveChild(0)
account_0_non_change_node = account_0_node.deriveChild(0)
account_0_address_0_node = account_0_non_change_node.deriveChild(0)

With this example is it obvious to see that calling getAddress on any HD node instance other than account_0_address_0_node would result in a violation of the BIP44 implementation.

So, where did we go wrong? Our misstep occurred (I think) when we started to imagine how this implementation could be adapted for use with Qi. There is a starkly different requirement of Qi wallets in that they must support aggregated signatures by many private keys. This requirement alone violates the initial HD wallet implementation which only contains a single public/private key pair per HD node instance. I see two pretty clear options to move forward here: scrap the node base implementation and allow people to only specify either full paths or path missing only address_index or we create another layer of abstraction above the QiHDWallet that manages deriving nodes properly and then uses the private keys of each leaf node for performing signature aggregation.

@alejoacosta74 alejoacosta74 force-pushed the ft/quaiwallet-getaddress-rebase branch from f37ad6a to 0060cc6 Compare May 23, 2024 16:38
@rileystephens28 rileystephens28 merged commit c3cf14e into dominant-strategies:master May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants